Skip to content

[turbopack] Use an arena for JSValues#94297

Open
sampoder wants to merge 3 commits into
canaryfrom
sp/turbopack/analyzer-arena-jsvalue
Open

[turbopack] Use an arena for JSValues#94297
sampoder wants to merge 3 commits into
canaryfrom
sp/turbopack/analyzer-arena-jsvalue

Conversation

@sampoder
Copy link
Copy Markdown
Member

@sampoder sampoder commented Jun 1, 2026

This PR takes the arena introduced in #94296 and uses it! It's large but shouldn't change functionality beyond the use of the arena. Some syntax related to pattern-matching had to change because bumpalo's Box doesn't support the same pattern matching that the standard library's Box does but the code should be equivalent.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Stats skipped

Commit: d72a407
View workflow run

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Failing test suites

Commit: d72a407 | About building and testing Next.js

pnpm test-start test/production/app-dir/metadata-static-route-cache/metadata-static-route-cache.test.ts (job)

  • app dir - metadata static routes cache > should generate different content after replace the static metadata file (DD)
Expand output

● app dir - metadata static routes cache › should generate different content after replace the static metadata file

next already started

  65 |   ) {
  66 |     if (this.childProcess) {
> 67 |       throw new Error('next already started')
     |             ^
  68 |     }
  69 |
  70 |     this._cliOutput = ''

  at NextStartInstance.start (lib/next-modes/next-start.ts:67:13)
  at Object.start (production/app-dir/metadata-static-route-cache/metadata-static-route-cache.test.ts:17:16)

pnpm test-start test/production/create-next-app/package-manager/pnpm.test.ts (job)

  • create-next-app with package manager pnpm > should use pnpm for --use-pnpm flag with example (DD)
  • create-next-app with package manager pnpm > should use pnpm when user-agent is pnpm with example (DD)
Expand output

● create-next-app with package manager pnpm › should use pnpm for --use-pnpm flag with example

Command failed with exit code 1 (EPERM): node /__w/next.js/next.js/packages/create-next-app/dist/index.js use-pnpm-with-example --use-pnpm --example https://github.com/vercel/next.js/tree/canary/examples/basic-css

  159 |     await useTempDir(async (cwd) => {
  160 |       const projectName = 'use-pnpm-with-example'
> 161 |       const res = await run(
      |                   ^
  162 |         [projectName, '--use-pnpm', '--example', FULL_EXAMPLE_PATH],
  163 |         nextTgzFilename,
  164 |         { cwd }

  at makeError (../node_modules/.pnpm/execa@2.0.3/node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/.pnpm/execa@2.0.3/node_modules/execa/index.js:112:26)
  at production/create-next-app/package-manager/pnpm.test.ts:161:19
  at useTempDir (lib/use-temp-dir.ts:24:5)
  at Object.<anonymous> (production/create-next-app/package-manager/pnpm.test.ts:159:5)

● create-next-app with package manager pnpm › should use pnpm when user-agent is pnpm with example

Command failed with exit code 1 (EPERM): node /__w/next.js/next.js/packages/create-next-app/dist/index.js user-agent-pnpm-with-example --example https://github.com/vercel/next.js/tree/canary/examples/basic-css

  177 |     await useTempDir(async (cwd) => {
  178 |       const projectName = 'user-agent-pnpm-with-example'
> 179 |       const res = await run(
      |                   ^
  180 |         [projectName, '--example', FULL_EXAMPLE_PATH],
  181 |         nextTgzFilename,
  182 |         {

  at makeError (../node_modules/.pnpm/execa@2.0.3/node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/.pnpm/execa@2.0.3/node_modules/execa/index.js:112:26)
  at production/create-next-app/package-manager/pnpm.test.ts:179:19
  at useTempDir (lib/use-temp-dir.ts:24:5)
  at Object.<anonymous> (production/create-next-app/package-manager/pnpm.test.ts:177:5)

pnpm test-start-turbo test/e2e/app-dir/navigation/navigation.test.ts (turbopack) (job)

  • app dir - navigation > hash > should scroll to the specified hash (DD)
Expand output

● app dir - navigation › hash › should scroll to the specified hash

expect(received).toBe(expected) // Object.is equality

Expected: false
Received: true

  197 |           url.includes('with-query-param')
  198 |         )
> 199 |         expect(hasQueryParamRscRequest).toBe(false)
      |                                         ^
  200 |       }
  201 |
  202 |       await checkLink('query-param', 2284)

  at Object.toBe (e2e/app-dir/navigation/navigation.test.ts:199:41)

@sampoder sampoder requested review from bgw and lukesandberg June 1, 2026 18:31
@sampoder sampoder marked this pull request as ready for review June 1, 2026 18:31
@sampoder sampoder force-pushed the sp/turbopack/analyzer-arena-infra branch from a98d014 to e21492c Compare June 1, 2026 23:29
@sampoder sampoder force-pushed the sp/turbopack/analyzer-arena-jsvalue branch from 4b5a6ea to 9c87cd9 Compare June 1, 2026 23:29
Comment thread crates/next-core/src/segment_config.rs Outdated
Comment on lines 115 to +116
#[derive(Debug)]
pub enum Effect {
pub enum Effect<'a> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up: Maybe also make Effect use the arena for everything? Right now it has some global-heap-allocated types.

};

enum EarlyReturn {
enum EarlyReturn<'a> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible future PR: Also use an arena here

Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/graph/visitor.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/graph/visitor.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/graph/visitor.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/graph/visitor.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/mod.rs Outdated
Base automatically changed from sp/turbopack/analyzer-arena-infra to canary June 3, 2026 21:43
sampoder added a commit that referenced this pull request Jun 3, 2026
…#94296)

This PR implements `BumpVec` this is a custom vector implementation that
takes in a `Bump` as a type (from bumpalo) so that we get `Send`/`Sync`
on our `JSValue` vectors when we make that switch in:
#94297.
@sampoder sampoder force-pushed the sp/turbopack/analyzer-arena-jsvalue branch 2 times, most recently from 9b57fe5 to b83e7c6 Compare June 3, 2026 22:06
Bump-allocates the analyzer's `JsValue` tree into a per-thread arena
(`ThreadLocal<Bump>`) that lives for the duration of
`analyze_ecmascript_module_internal` and is freed in one shot when it returns,
replacing the previous `Arc`/`Box`/`Vec` heap allocations. `JsValue` becomes
`JsValue<'a>`, with its list children stored in `BumpVec` (growable) or
`bumpalo::boxed::Box<[_]>` (fixed) and scalar children in `bumpalo::boxed::Box`.

Sync functions take `&Bump` and allocate directly; the async (`Send`-bound)
analysis pipeline holds a `&ThreadLocal<Bump>` and obtains a `&Bump` via
`get_or_default()`, so no `!Send` `&Bump` is ever held across an `.await`.

Builds on the `BumpVec` introduced in the previous commit.
@sampoder sampoder force-pushed the sp/turbopack/analyzer-arena-jsvalue branch from b83e7c6 to 4cc4b5b Compare June 3, 2026 22:09
@sampoder sampoder requested a review from bgw June 3, 2026 22:10
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/normalize.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/normalize.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/normalize.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/normalize.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/builtin.rs
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/builtin.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/builtin.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/builtin.rs Outdated
Comment thread turbopack/crates/turbopack-ecmascript/src/analyzer/builtin.rs Outdated
@sampoder sampoder force-pushed the sp/turbopack/analyzer-arena-jsvalue branch from 5610ae9 to 916162c Compare June 4, 2026 08:32
@sampoder sampoder requested a review from bgw June 4, 2026 16:30
Copy link
Copy Markdown
Contributor

@lukesandberg lukesandberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be useful to describe the thread_local! design in the PR description, how this is solving a problem with async and the associated risks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants